Skip to content

8354679: [CRaC] jdk.crac.management makes JdkManagementCheckSince fail #225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

TimPushkin
Copy link
Collaborator

@TimPushkin TimPushkin commented Apr 16, 2025

Fixes the failing test, by using @since CRaC in CRaC docs and --ignoreSince in since-checker tests which check them.

A since-checker test for jdk.crac module is also added but it is disabled until JDK-8354921 is fixed.

Also adds the since-checking tests to CI.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8354679: [CRaC] jdk.crac.management makes JdkManagementCheckSince fail (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/225/head:pull/225
$ git checkout pull/225

Update a local copy of the PR:
$ git checkout pull/225
$ git pull https://git.openjdk.org/crac.git pull/225/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 225

View PR using the GUI difftool:
$ git pr show -t 225

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/225.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 16, 2025

👋 Welcome back tpushkin! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Apr 16, 2025

@TimPushkin This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8354679: [CRaC] jdk.crac.management makes JdkManagementCheckSince fail

Reviewed-by: rvansa

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the crac branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@rvansa) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2025
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2025

Webrevs

@rvansa
Copy link
Collaborator

rvansa commented Apr 17, 2025

I think that it's OK to add the symbols to 24 only; CRaC versioning can be considered orthogonal to upstream JDK versioning. At this stage we keep backward compatibility for users' convenience, not as a rule. Testing API compatibility is useful as CRaC'ed JDK should be 100% usable as a drop-in replacement for upstream JDKs.

If jdk.crac can't be addressed, we can leave it out of scope. However could you file a bug for SinceChecker in upstream?

Last but not least; please outline the steps (can be just a comment on this PR) what will we have to do when we rebase on top of JDK 26.

@TimPushkin
Copy link
Collaborator Author

However could you file a bug for SinceChecker in upstream?

Filed the SinceChecker bug as JDK-8354921.

Last but not least; please outline the steps (can be just a comment on this PR) what will we have to do when we rebase on top of JDK 26.

Whenever we merge an update of the symbols from the mainline (can happen multiple times between a rampdown and the respective GA) we'll need to first overwrite our symbols with the incoming changes and then run make/scripts/generate-symbol-data.sh to update the symbols with CRaC's ones.

Whenever we add a method to the public CRaC Java API we also need to run the script to update the symbols.

I also found JDK-8345212, which we don't yet have in this fork. It should allow us to continue using @since TBD (or @since CRaC, or whatever) and the test will pretend like the CRaC API has been added in the current version — we won't need to do the symbols updates. @rvansa WDYT?

@rvansa
Copy link
Collaborator

rvansa commented Apr 22, 2025

Whenever we merge an update of the symbols from the mainline (can happen multiple times between a rampdown and the respective GA) we'll need to first overwrite our symbols with the incoming changes and then run make/scripts/generate-symbol-data.sh to update the symbols with CRaC's ones.

OK, I thought that we could somehow preserve the oldest CRaC JDK version where the symbol appeared. But we don't need to change the taglet on too many places, so let's use this as is.

I also found JDK-8345212, which we don't yet have in this fork. It should allow us to continue using @SInCE TBD (or @SInCE CRaC, or whatever) and the test will pretend like the CRaC API has been added in the current version — we won't need to do the symbols updates.

That sounds useful, but we don't need to merge changes out-of-band, this PR is good as it is. Let's integrate this and use the -ignoreSince the next time this code needs updating anyway.

@TimPushkin
Copy link
Collaborator Author

OK, I thought that we could somehow preserve the oldest CRaC JDK version where the symbol appeared. But we don't need to change the taglet on too many places, so let's use this as is.

Symbols are fixed after GA so after GA they'll be preserved. But between rampdown and GA we'll need to be updating them (only the version that has not yet been GA-ed) when they are updated upstream.

Example for JDK 26:

  1. 25 rampdown happens, 26 is created and symbols for 25 along with it — when merging this, we'll take the new 25 symbols and run the script to update them from the last 25-CRaC build.
  2. 25's symbols get updated — when merging this, we'll overwrite our 25's symbols with the incoming ones, then run the script again to update them with the last 25-CRaC build (the same build as in step one), then fixup the symbols manually to remove the changes unrelated to CRaC (there will be such changes because the last 25-CRaC build will be based on an older 25 than the updated symbols). This step can happen a few times.
  3. 25 GA happens — after this moment 25's symbols are fixed, we won't need to touch them ever again.

That sounds useful, but we don't need to merge changes out-of-band, this PR is good as it is. Let's integrate this and use the -ignoreSince the next time this code needs updating anyway.

I was proposing to wait until that change gets merged-in. I believe we need to decide now: we either merge this PR and start generating CRaC symbols from now on or wait for the SinceChecker update that should allow us to continue without generating the symbols (I actually need to try this locally by cherry-picking to see how it works before we decide). It will be weird if now we start generating the symbols but then we suddenly stop.

@TimPushkin
Copy link
Collaborator Author

Oh, actually, I believe we are still merging 25 updates that occurred before 24 GA, so we'll go through the steps 2 and 3 above even for 24's symbols.

@bridgekeeper
Copy link

bridgekeeper bot commented May 20, 2025

@TimPushkin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@TimPushkin
Copy link
Collaborator Author

Still active. I need to check if JDK-8345212 will work as expected and then we'll decide whether we want to go that way or the way this PR was originally done.

@TimPushkin TimPushkin marked this pull request as draft May 20, 2025 07:43
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 20, 2025
@TimPushkin
Copy link
Collaborator Author

JDK-8345212 works as expected. We will be able to just put @since CRaC to our package-infos and module-infos and add --ignoreSince CRaC to SinceChecker tests that touch CRaC sources.

I think this approach is better because we won't have to update the symbols on every release (possibly multiple times) and it seems to be the accepted way for long-running non-mainline projects (this is the reasoning in JDK-8345212).

JDK-8345212 will come with jdk-25+10, we are now at jdk-25+4. I'll leave this PR as a draft until then. After that I'll change it to satisfy JDK-8345212.

@rvansa
Copy link
Collaborator

rvansa commented May 27, 2025

@TimPushkin 25+10 was just merged.

@TimPushkin
Copy link
Collaborator Author

We should now use @since CRaC in CRaC docs and add --ignoreSince CRaC to since-checker tests of modules containing documented CRaC code.

I also added a disabled since-checker test for jdk.crac module. It can be enabled after JDK-8354921 is fixed.

Since-checker will also be run in our CI.

@TimPushkin TimPushkin marked this pull request as ready for review May 27, 2025 12:31
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 27, 2025
@TimPushkin
Copy link
Collaborator Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label May 27, 2025
@openjdk
Copy link

openjdk bot commented May 27, 2025

@TimPushkin
Your change (at version e690c7d) is now ready to be sponsored by a Committer.

@rvansa
Copy link
Collaborator

rvansa commented May 28, 2025

/sponsor

@openjdk
Copy link

openjdk bot commented May 28, 2025

Going to push as commit bcb3924.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 28, 2025
@openjdk openjdk bot closed this May 28, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 28, 2025
@openjdk
Copy link

openjdk bot commented May 28, 2025

@rvansa @TimPushkin Pushed as commit bcb3924.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@TimPushkin TimPushkin deleted the since-checker branch May 29, 2025 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

2 participants